-
Notifications
You must be signed in to change notification settings - Fork 112
Fix srcToken error #1403
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix srcToken error #1403
Conversation
|
Hey @mdbraber , thanks for creating a potential fix for the issue. My concern right now is how it got as far as it did in the logic since #1402 implies that the logic actually ran for yaml key sort. The logic should not make it past this check: obsidian-linter/src/rules/yaml-key-sort.ts Lines 41 to 44 in cce4f98
If there is no YAML, then it should just skip the rule as a whole. So I am more worried that this is a bandaid that hides the reason that |
| if (doc.contents == null) { | ||
| return text; | ||
| } | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this not be before line 64 since that is where the error is happening since that allows us to exit immediately after we know there is no content in the YAML? At least, that is where the error happens for me locally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes - you're right. Or should the code of
obsidian-linter/src/utils/yaml.ts
Line 31 in 1877e2b
| if (!yaml) { |
if (!yaml || yaml[1] == "")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Returning an empty YAML string is fine from my understanding since it is possible to have YAML that is just empty and I would expect the content to be an empty string in that case. I am not sure if things would break if we also returned null for empty strings for the YAML.
I think that the proposed solution just before getting an empty document makes the most sense to me at this time.
|
It has been a little a while on this. I am going to just go ahead and merge this since it looks to fix the issue. |
|
Thanks Peter! |
Fixes #1402 @pjkaufman could you review?